feat: add log aggregation and request correlation for Playwright tests #298
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mxschmitt
force-pushed
the
log-improvements
branch
5 times, most recently
from
e702c16 to
31aba4e
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive logging and correlation system for Playwright tests, enabling backend logs to be aggregated and correlated with test reports through unique identifiers.
Changes:
- Implemented a new log-aggregator service for centralized log collection and querying
- Added requestID and testID fields throughout the system for log correlation
- Enhanced all services (control, file, worker) to capture and forward logs with correlation IDs
- Updated e2e tests to automatically attach aggregated logs to test reports
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| worker-python/Dockerfile | Standardized Dockerfile AS keyword capitalization |
| worker-javascript/Dockerfile | Standardized Dockerfile AS keyword capitalization |
| worker-java/Dockerfile | Standardized Dockerfile AS keyword capitalization |
| worker-csharp/Dockerfile | Standardized Dockerfile AS keyword capitalization |
| log-aggregator/main.go | New in-memory log aggregation service with HTTP API |
| log-aggregator/Dockerfile | Dockerfile for building the log-aggregator service |
| k8/log-aggregator-service.yaml | Kubernetes service configuration for log-aggregator |
| k8/log-aggregator-deployment.yaml.tpl | Kubernetes deployment template for log-aggregator |
| k8/file-deployment.yaml.tpl | Added LOG_AGGREGATOR environment variables |
| k8/control-deployment.yaml.tpl | Added LOG_AGGREGATOR environment variables |
| internal/workertypes/types.go | Added RequestID and TestID fields to worker request/response payloads |
| internal/worker/worker.go | Integrated structured logging with log aggregation |
| internal/logagg/logagg.go | Core log aggregation client library |
| internal/logagg/hook.go | Logrus hook for automatic log forwarding |
| file-service/main.go | Added request-scoped logging with correlation IDs |
| file-service/Dockerfile | Updated to copy full internal directory |
| e2e/tests/visual.spec.ts | Added log attachment functionality |
| e2e/tests/api.spec.ts | Added log attachment with correlation IDs |
| control-service/workers.go | Updated worker to pass correlation IDs |
| control-service/main.go | Enhanced with request-scoped logging and correlation |
| control-service/Dockerfile | Standardized Dockerfile AS keyword capitalization |
| .github/workflows/nodejs.yml | Configured log-aggregator in CI pipeline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+162
to
+163
| c.Response().Header().Set("X-Request-ID", requestID) | ||
| c.Response().Header().Set("X-Test-ID", testID) |
There was a problem hiding this comment.
The X-Request-ID and X-Test-ID headers are already set at lines 162-163. This duplicate header setting is redundant and could be removed to improve code maintainability.
Suggested change
| c.Response().Header().Set("X-Request-ID", requestID) | |
| c.Response().Header().Set("X-Test-ID", testID) |
mxschmitt
force-pushed
the
log-improvements
branch
from
cfe2d94 to
d63519e
Compare
mxschmitt
changed the title
Critical fixes: - Replace deprecated io/ioutil with io.ReadAll in control-service - Fix race condition in worker logger by using instance logger instead of modifying global logger - Add error handling for http.ResponseWriter.Write() in log-aggregator Medium priority improvements: - Create custom HTTP client with proper timeouts in logagg package to ensure context cancellation is respected - Configure connection pooling and timeout settings for better reliability This ensures: 1. Go 1.16+ compatibility (no deprecated APIs) 2. Thread-safe logging in worker processes 3. Proper error reporting when writing log responses fails 4. HTTP requests properly respect context deadlines Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Technical Details
log-aggregator- lightweight Go HTTP service with 60min TTL in-memory storageinternal/logagg- shared library for posting logs to aggregator (best-effort, 2s timeout)/logs/{testId}endpoint and attach to test reportsChanges by Service
Infrastructure
LOG_AGGREGATOR_URL,LOG_AGGREGATOR_ENABLEDAS builder)🤖 Generated with Claude Code